feat: auto failover for API with LK Cloud - #936
Conversation
automatically fail over 5xx and transport errors
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // discover regions lazily | ||
| if !fetchedRegions { | ||
| settings = t.regions.get(originalScheme, originalHost, req.Header) |
There was a problem hiding this comment.
Should the discovery get/fetch respect the caller's context? Right now RoundTrip calls t.regions.get(originalScheme, originalHost, req.Header) without req.Context(), and fetch uses http.NewRequest on the 5s-timeout client — so it ignores the caller's deadline/cancel.
There was a problem hiding this comment.
it shouldn't.. this is an internal request to discover regions, we'll limit it to a much shorter timeout. so for example, if someone was doing a CreateSIPParticipant with a 30s timeout, we do not want region refresh to take the same 30s.
There was a problem hiding this comment.
Totally agree the discovery fetch shouldn't ride the caller's 30s — capping it short is right. One clarification though: threading the context doesn't undo that cap. http.NewRequestWithContext(ctx, …) on the Timeout: 2s client is bounded by min(2s, remaining deadline), not the max - the client derives its deadline from the request ctx, so whichever fires first wins - right? Your CreateSIPParticipant/30s case still gets capped at 2s; the 30s never applies.
The reason to still pass ctx is the two cases the fixed 2s alone doesn't cover:
- Cancellation - if the caller cancels (client disconnected, op aborted, process draining), the in-flight discovery keeps burning up to 2s on an abandoned request instead of stopping right away.
- Sub-2s deadlines (maybe that's not too frequent in practice - but theoretical possibility) — a caller with a 500ms timeout: the primary fails at 500ms, then discovery still runs up to 2s, so the call returns ~2.5s and the result gets discarded anyway.
There was a problem hiding this comment.
great points on both, I've added the following:
- if request timeout is under 5s, then we do not retry (too short of timeout)
- figure out a timeout per request, instead of inheriting the context for subsequent retries
|
just for my education: are all APIs idempotent and safe to be retried on 5xx? |
|
related to my comment above - just thinking out loud here |
| client := lksdk.NewRoomServiceClient(testServerURL(t), "devkey", "secret") | ||
| ctx := failoverCtx(t, lksdk.FailoverOn, map[string]string{hdrFailRegions: "0"}) | ||
| _, err := client.CreateRoom(ctx, &livekit.CreateRoomRequest{Name: "api-test"}) | ||
| require.NoError(t, err, "should fail over to a healthy region") |
There was a problem hiding this comment.
can this test the exact region that it connects to? Guess not as CreateRoomResponse does not have a region indication.
In this case, all requests should be retried. our server should be as idempotent as possible. that would be the right design patterns here. otherwise, if a request returns 500, how should we react to it? the contract says the API errored out and the most natural thing for any end user to handle is to retry it. for observability, we should be able to see the HTTP path to determine the method/etc. |
|
@milos-lk updated the implementation to address feedback. now we have a consolidated region cache. |
We tell our customers to retry on 5xx without any extra caveat. We should solve any bug that make 5xx retries no idempotent if we find any. Timeouts particularly could be problematic (ie StartEgress?) |
5 participants
automatically fail over 5xx and transport errors